-
Notifications
You must be signed in to change notification settings - Fork 51
Decentralization of configuration parameters phase 1 - Aggregator local parameters #2736
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
turmelclem
wants to merge
15
commits into
main
Choose a base branch
from
ctl/2692-decentralization-of-configuration-parameters-phase-1-local-parameters
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Decentralization of configuration parameters phase 1 - Aggregator local parameters #2736
turmelclem
wants to merge
15
commits into
main
from
ctl/2692-decentralization-of-configuration-parameters-phase-1-local-parameters
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test Results 4 files ± 0 168 suites ±0 24m 6s ⏱️ +14s Results for commit df320c5. ± Comparison against base commit c7220be. This pull request removes 3 and adds 8 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
407ea74 to
bc2e5d5
Compare
mithril-aggregator/src/dependency_injection/builder/enablers/mithril_network_configuration.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/dependency_injection/builder/enablers/mithril_network_configuration.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/dependency_injection/builder/enablers/mithril_network_configuration.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/dependency_injection/builder/enablers/mithril_network_configuration.rs
Fixed
Show fixed
Hide fixed
120452b to
4e21205
Compare
1b74219 to
890ad0b
Compare
07d14a3 to
80abe55
Compare
ea59e83 to
de20459
Compare
…local configuration implementation
…onfiguration and adapt epoch service
…leanup dependency injection
de20459 to
2097da6
Compare
…gning_config optionnal (mandatory for leader)
Rework `init_state_from_fixture` to not save epoch_settings and works with the fixed window of three epoch (aggregate/next aggregate/signer registration), epoch settings should already exists, most of the time they will be inserted by the handle discrepancies system
- run it at the end of the serve dependency container build - retrieve and save data from the network configuration provider instead of the local node configuration - update follower integration test to check that local protocol parameter configuration is not read, instead the configuration is read through the network configuration provider from the leader
Since now the follower read the network config from the leader, this means that the update of the protocol parameters is now a responsability of the leader only. This lead to flakiness because this step was restarting all aggregators, and sometimes the follower started before the leader and had a error when it executed its handle discrepencies because the leader http server was still down.
until we can figure out how to make the logs group work correctly when the e2e is retry by `nick-fields/retry` (currently the group before the retry are lost)
2097da6 to
6319f63
Compare
…ction between local configuration and network configuration For leader aggregator this does not change anything right now since both value come from the aggregator configuration. For follower this allow them to use a subset of the signed entity types allowed in their leader.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Content
This PR includes a rework of the retrieval of the network configuration parameters in the
mithril-aggregatorin order to prepare their decentralization.Details
Mithril-aggregator
Changes on both leader and follower:
EpochServiceto use theMithrilNetworkConfigurationProvidersystem frominternal/mithril-protocol-configinform_epochnow fetch its current/next/registration AggregatorEpochSettings from the configuration providerupdate_epoch_settingsthat is run by the state machine after runninginform_new_epoch, it's now done withininform_epochright after retrieving the parameters from the configuration provider and at an offset of +1. This means that the parameters are now stored at the last moment right before their usage instead of ahead of time.insert or ignoreinstead of aninsert or replacewhen storing the epoch settings. Now stored epoch settings are considered final, that's why we moved the time of their insertion to right before their usage.handle_discrepancies_at_startupwith the aim of making it ready for decentralization:MithrilNetworkConfigurationProviderinstead of the aggregator configurationServeCommandDependenciesContainerinstead of after building the epoch setting store. This changes limits its call to theservecommand, previously other commands could call it even if they did not need it at all.protocol_parametersandcardano_transactions_signing_configare now options, but still mandatory for a leader aggregator (the missing configuration error is now handled manually instead of automatically by theconfigcrate)preload_security_parameterwith a default value of2160. Used to configure the transactions preloader instead of fetching the security parameter in thecardano_transactions_signing_configconfiguration.create_certificate_followerintegration test by making it check that the follower aggregator works without configuredprotocol_parametersServeCommandDependenciesContainer:epoch_settingstable, but only in thesignerandsigner_registrationtable. This means that theepoch_settingsmust have been filled beforehand (either by running handle discrepancies or manually).Leader aggregator specific:
LocalMithrilNetworkConfigurationProvider: aMithrilNetworkConfigurationProviderthat fetch its data first from theepoch_settingstable in the sqlite database, and if an entry is missing for an epoch, it fallback to the usual configuration parameters (protocol_parametersandcardano_transactions_signing_config)Follower aggregator specific:
mithril-protocol-config::http::HttpMithrilNetworkConfigurationProvideras its network configuration provider, fetching data from its configured leader aggregatorMithril-end-to-end
update_protocol_parametersstep leader only. Now it doesn't matter for the follower since it retrieve its configuration from the leader and no longer read its configuration, and keeping it introduced a flakiness since sometimes the follower aggregator restarted before the leader could restart its http server, making the handle discrepancies of the follower fails (since it do a call to the configuration provider).::group::when running in github action. Disabled for now as there's an remaining issue to tackle first: when the e2e is retry the logs in those groups for the previous iteration are missing in the action output.Pre-submit checklist
Comments
There's a know issue for the updated
handle_discrepancies_at_startup: it run twice.This is because the
ServeCommandDependenciesContaineris also built twice, once for its purpose, a second time for the http server.This is harmless as this will record twice the same epoch settings and the second recording will be ignored, but we should probably construct a
HttpServerDependenciesContainerinstead of reusing the one of the serve command.Issue(s)
Relates to #2692